I don't quite follow. If I say, for example, halt-height == 1000, then when Commit is called indicating block completion, the process will exit after 1000. How is it non-deterministic? Why is the logic duplicated in FinalizeBlock?

because sending signal to self process is asynchronous, the current process can run for a while before the signal is handled and stop the node, I seems to observe that when I use --halt-height with local blocks that it don't stop at the expected height exactly.

Mhhh, so os.Exit() is async?? Or do you mean that os.Exit() actually sends a signal, which is caught in the server package, which case that is obviously async, yes.

Mhhh, so os.Exit() is async?? Or do you mean that os.Exit() actually sends a signal, which is caught in the server package, which case that is obviously async, yes.

I think it only calls os.Exit if signal sending fails

I'm sorry, I don't follow. When the halt-height is reached, os.Exit() will be called:

https://github.com/cosmos/cosmos-sdk/blob/v0.47.3/baseapp/abci.go#L479-L485

I'm sorry, I don't follow. When the halt-height is reached, os.Exit() will be called:

https://github.com/cosmos/cosmos-sdk/blob/v0.47.3/baseapp/abci.go#L479-L485

You can check the internal of halt method, it don't call os.Exit unconditionally

You can check the internal of halt method, it don't call os.Exit unconditionally

I don't understand what this means. os.Exit() is called when the halt-height is reached, immediately exiting the process AFAIU.

You can check the internal of halt method, it don't call os.Exit unconditionally

I don't understand what this means. os.Exit() is called when the halt-height is reached, immediately exiting the process AFAIU.

func (app *BaseApp) halt() {
	app.logger.Info("halting node per configuration", "height", app.haltHeight, "time", app.haltTime)

	p, err := os.FindProcess(os.Getpid())
	if err == nil {
		// attempt cascading signals in case SIGINT fails (os dependent)
		sigIntErr := p.Signal(syscall.SIGINT)
		sigTermErr := p.Signal(syscall.SIGTERM)

		if sigIntErr == nil || sigTermErr == nil {
			return
		}
	}

	// Resort to exiting immediately if the process could not be found or killed
	// via SIGINT/SIGTERM signals.
	app.logger.Info("failed to send SIGINT/SIGTERM; exiting...")
	os.Exit(0)
}

os.Exit() is not called if signal sending is successful, it's a fallback if signal sending failed, it's not ideal because the shutdown would not be graceful.

Wow I didn't see the PID logic -- completely forgot about that. I believe I originally wrote this code, but I can't recall why not just resort to os.Exit() always?

Wow I didn't see the PID logic -- completely forgot about that. I believe I originally wrote this code, but I can't recall why not just resort to os.Exit() always?

maybe for graceful shutdown, although right now we don't have critical stuff to cleanup, but for async commit (memiavl is doing that already), it'll be helpful to wait for it to complete before shutdown.
And I'm not sure if the Commit event of the last block should return successfully for cometbft to finish some post logic.

LGTM and thank you @yihuang! Would be nice though to have a test to lock-in this behavior to prevent future regressions. It is critical code yet tomorrow I can sneakishly delete it and no tests will fail.

no problem, I just feel the solution is not ideal yet, first of all, there are code duplication between Commit and BeginBlocker, and x/upgrade has done something similar just by panic, so I think maybe just do the panic in BeginBlocker would be good enough, aka. remove the signal sending and os.Exit stuff from Commit? WDYT @alexanderbez

LGTM and thank you @yihuang! Would be nice though to have a test to lock-in this behavior to prevent future regressions. It is critical code yet tomorrow I can sneakishly delete it and no tests will fail.

no problem, I just feel the solution is not ideal yet, first of all, there are code duplication between Commit and BeginBlocker, and x/upgrade has done something similar just by panic, so I think maybe just do the panic in BeginBlocker would be good enough, aka. remove the signal sending and os.Exit stuff from Commit? WDYT @alexanderbez

Yes, a panic in BeginBlock is perfectly reasonable IMO. Just make sure it's triggered at HaltHeight + 1

Yes, a panic in BeginBlock is perfectly reasonable IMO. Just make sure it's triggered at HaltHeight + 1

done, I think the equivalence of panic in latest abci is just simply returning errors.